Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add yamux support #2397

Merged
merged 1 commit into from
Feb 6, 2024
Merged

feat: add yamux support #2397

merged 1 commit into from
Feb 6, 2024

Conversation

richard-ramos
Copy link
Member

Description

Added support to yamux. Since the order of multiplexers matter, it's added after mplex so it continues being the default multiplexer (instead of completely removing mplex, as suggested in the og issue). This is required since go-libp2p is removing support for mplex.

Closes #2331

Copy link

github-actions bot commented Feb 5, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2397

Built from 211348f

Copy link
Contributor

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

CI green but I still have doubts.

In theory nim-libp2p should use Yamux for go-libp2p interop and fall back to Mplex if needed right?

Does this need to be tested more? Maybe I'm paranoid :P

@richard-ramos
Copy link
Member Author

In theory nim-libp2p should use Yamux for go-libp2p interop and fall back to Mplex if needed right?

I tried using wakunode2 and it could connect succesfully against peers that support only yamux or only mplex, or both!

@chaitanyaprem
Copy link
Contributor

Since the order of multiplexers matter, it's added after mplex so it continues being the default multiplexer

If a peer supports both mplex and yamux, then yamux would be used right?
mplex would only be used if a peer doesn't support yamux.

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for it! Just some naïve questions :)

How the stream multiplexer is being picked up? Does it first try to use mplex and if the other peer doesn't support it, then it tries with yamux?

@richard-ramos
Copy link
Member Author

@chaitanyaprem @Ivansete-status
If i understood correctly the code in https://github.com/status-im/nim-libp2p/blob/eb0890cd6f5d36e1b3ae414bffda59d31dfc2e7c/libp2p/upgrademngrs/muxedupgrade.nim#L43-L45 nim-libp2p will try muxers depending on the order they're defined both in identify protocol and when building the switcher, so it is like @Ivansete-status says: it will attempt to use mplex first, and if not available, then yamux.

So with the changes of this PR, if for ex, if go-waku returns via identify protocol the muxers protocol ids in this order: [yamux_protocol_id, mplex protocol_id], nim-libp2p will use yamux first. But if go-waku were to return [mplex_protocol_id, yamux_protocol_id], since in the switcher mplex is used as default (we called withMplex before withYamux), then it will prefer to use mplex over yamux.

We could switch the order, but i went with mplex by default, since that's what nwaku nodes out there in the wild are using, and eventually just use yamux?

@richard-ramos richard-ramos merged commit 1b40266 into master Feb 6, 2024
9 of 10 checks passed
@richard-ramos richard-ramos deleted the feat/yamux branch February 6, 2024 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore: use yamux instead of mplex
4 participants